fix: version comparison fails closed instead of fabricating update notices - #155

* chore(plan): read installed versions from the npx/uv runner caches (#150)

Sixth attempt. Research killed the naive form first: every manifest server
launches via npx/uvx, which are EPHEMERAL RUNNERS -- 'npm ls @upstash/context7-mcp'
returns (empty), so shelling out to npm ls / uv pip show would have silenced
every notice, exactly the failure mode of attempt 4.

The runner CACHES are queryable, and that is the source all five prior attempts
lacked: each ~/.npm/_npx/<hash>/package.json names its package and node_modules
holds the resolved version (verified: @brightdata/mcp 2.11.1, @eslint/mcp
0.3.8), and uv's environments-v2 envs carry .dist-info metadata. Bound to
package identity by construction, unlike descriptions_cache.version (an upstream
snapshot) or serverInfo.version (an implementation version).

Cost measured: 0.36s for 114 npx entries, filesystem only, no subprocess.
Rests on #155, which made the comparator fail closed.

* chore(plan): record why matching on dependencies cannot hit a transitive

Sampled six npx cache entries: each declares exactly ONE dependency (the
invoked package) while node_modules holds the transitive closure -- 212
packages in one entry. So matching must be against 'dependencies'; matching
node_modules would resolve any transitive package. Checked before writing code
because it is the load-bearing assumption of the npm probe.

* chore(plan): record board rejection of the runner-cache plan (3/3 DISAGREE)

B1 (fatal): the plan kept descriptions_cache.version as a fallback while its own
acceptance criteria required silence in that case. That value is an upstream
snapshot (refresher.py:243), so identity agreement cannot make it an observed
installed version -- the sixth attempt would still emit inferred notices. My
proposed test specified 'no cache entry AND no orderable fallback', so it would
have passed while the real regression went uncovered: the same test-shape
failure as rounds 1-4 of PR #154.

B2: neither probe binds a CONFIGURED SERVER to the cache entry it runs. Verified
on this machine: the npx cache holds @consiliency/agent-board-mcp at 1.2.0,
1.2.1 AND 1.2.2 concurrently, so newest-by-mtime can pick an unrelated
invocation -- including update_server's own pkg@latest probe. uv is worse: an
env holds its full transitive closure (30 dist-infos observed) with no
single-declared-dependency marker, so a name scan matches transitives.

The caches do hold real installed versions -- that research stands. What they
lack is server->entry provenance. Recorded three candidate directions.

* chore(plan): record the installed version at spawn, plus a staleness guard (#150)

Seventh attempt. Maintainer decision after the runner-cache plan was rejected
3/3: record at spawn, plus a periodic guard so a long-running server does not
keep a stale baseline forever.

Spawn-time capture differs from all six prior attempts because provenance is by
CONSTRUCTION rather than inference: at manager.py:1365 pmcp calls
create_subprocess_exec with an argv it chose, so there is no scan, no
mtime heuristic, and no ambiguity about which cache entry is in play. Attempt 6
failed precisely there -- verified the npx cache holds one package at 1.2.0,
1.2.1 and 1.2.2 concurrently.

The maintainer's guard requirement is confirmed as a real gap: the existing
hourly sweep re-checks UPSTREAM latest, but a spawn-time value is captured only
at spawn, so a server left running for weeks keeps its baseline indefinitely.
The guard extends the existing _stale_indexer_loop rather than adding new
scheduling, and re-reads from disk without ever respawning a running server.

Carries the board's B1 finding forward: NO fallback to descriptions_cache.version
(an upstream snapshot), and the test for that case now includes an orderable
fallback, which the last plan's test omitted.

* chore(plan): record board rejection of the spawn-provenance plan

B1 (fatal): 'argv is authoritative provenance' is false. create_subprocess_exec
returns once NPX starts, before it has selected/fetched/launched the package,
and npm may resolve to a local, global or cache-backed copy -- only one branch
produces an _npx dir. Verified independently before the verdicts: a live
'npx -y @eslint/mcp' tree exposes NO _npx path in any descendant's cmdline, cwd
or fds. Knowing the argv tells pmcp what it ASKED FOR, not what npm CHOSE.

B2: the plan changed only the 'current' source. Both notice writers still take
'latest' from _get_server_config_for_update, which prefers the manifest and
ignores the .mcp.json override -- so B's installed version can still be compared
against A's upstream latest, which is the original #150 defect intact.

Seven attempts have now failed at the same joint: pmcp cannot observe which
package artifact is actually executing for a running server.

* chore(plan): ask the server its own version (#150, eighth attempt)

Maintainer decision after the spawn-provenance plan was rejected. Running the
package's own --version is the only method that observes the artifact that
actually runs, because the artifact answers for itself.

Measured against 12 real manifest servers before planning, not assumed:
  6/12 (50%) parsed a version
  @circleci/mcp-server-circleci IGNORED the flag and hit the 90s cap
  @eslint/mcp printed 'server is running' and EXITED 0 -- exit status is not a
    usability signal
  firecrawl-mcp emitted only npm noise containing koa-router@14.0.0, exactly
    what a permissive regex would grab as a version
Strict whole-line matching verified to extract 4.0.2 and 0.0.79 while rejecting
both bad cases. Median latency 5s, max 90s -> a 20s timeout and bounded
concurrency are load-bearing, and ~390s serial across 98 servers makes caching
required rather than an optimisation.

Carries both prior board findings forward: no fallback to
descriptions_cache.version (B1), and 'latest' must come from the same effective
config as the probe so an override cannot be compared against the manifest
package's latest (B2).

Open question recorded for the board: the probe STARTS the package, so a server
that ignores --version briefly runs with whatever side effects it has at
startup. If that is unacceptable the honest conclusion may be to remove the
feature rather than make it 50% correct.

* chore(plan): measure the probe's startup side effects -- 7 procs, 5 sockets

Probing @circleci/mcp-server-circleci --version (a server that ignores the flag)
spawns 7 processes and opens 5 network sockets, sustained until killed. That is
a real server starting and connecting outward, caused by an advisory update
check, for a server that returns no version anyway.

@eslint/mcp by contrast exits before 10s, so its cost is bounded.

Recorded because it is the strongest argument against this approach and the
maintainer asked for the board's view on exactly this point.

* chore(plan): board rejects the probe 3/3 and recommends removing the feature

Both readable legs answered the disqualifying question directly. grok:
'Automatic --version probes are disqualifying. Remove the automatic notices
rather than ship a 50%-correct scheduled spawn.' codex: 'It executes
third-party server code merely to produce an advisory.' Both rejected the
'probe only running servers' mitigation as relocating rather than removing the
hazard -- those servers are the ones most likely to hold live credentials.

Matches the measurement taken before the verdicts: one flag-ignoring server
spawned 7 processes and 5 sockets.

Separately, grok found and I verified a LIVE bug in shipped code:
_run_update_probe_command spawns without start_new_session and its 60s
wait_for abandons the child on timeout -- no kill, no process-group cleanup --
so today's gateway.update_server can orphan a whole process tree. Independent
of #150; to be filed separately.

* wip: reap the update probe's process tree on timeout (pre-test)

* test: prove the update probe reaps its process tree on timeout

* docs: CHANGELOG for the update-probe orphan fix

* fix: catch asyncio.TimeoutError so the probe is reaped on Python 3.10

asyncio.TimeoutError only became an alias of the builtin TimeoutError in 3.11.
This project supports 3.10, where 'except (TimeoutError, ...)' does not catch
what wait_for raises -- so the timeout escaped and _terminate_process_tree never
ran. The orphan fix silently did nothing on the oldest supported version.

CI caught it: test (3.10) failed while 3.11 and 3.12 passed, and the runner log
showed 'Terminate orphan process: pid (3919) (python)' -- my own regression test
leaking the processes it spawns. Reproduced locally on 3.10, then verified all
three versions pass with the fix.

* test: run the orphan proof in a child interpreter

The test must spawn real process trees -- a mock cannot demonstrate an orphan.
But doing that inside the pytest process perturbs state neighbouring tests
measure: tests/runtime/test_downstream_remote.py asserts on THIS process's open
socket count and on the process-global sse_starlette.AppStatus latch. My test
knocked over two different tests on CI's 3.10 lane while passing locally.

Moves the scenario into a subprocess, so the real-process proof survives without
touching the suite's shared state. Stdlib subprocess -- no test-runner plugin
and no new dependency for one test.

Verified: reverting the fix makes it report 'ORPHANED' (RED); with the fix
'REAPED'; and test_tools.py + test_downstream_remote.py together now pass 188/188
on 3.10, the combination that was failing.

1. _semver_key replaced by the semver library (new dep, zero transitive deps).
   Verified it rejects every case the hand-written regex was built to reject
   (leading zeros, empty identifiers, non-SemVer) and orders beta.2 < beta.11.

2. is_version_newer's docstring now warns that a bare `not is_version_newer(...)`
   is unsafe, and an AST-based test enforces it repo-wide -- proven to fire by
   removing the real guard in refresher.py.

3. All-numeric digest disambiguation. Fail-closed was tried and REVERTED: it
   broke CalVer, which #155 deliberately supports and pins with an existing
   test -- trading a live capability for a latent bug. The partner value
   disambiguates instead. Also fixed a real bug found on the way: the digest
   pattern demanded a hex letter even when the `sha256:` prefix was present,
   rejecting all-numeric prefixed digests.

4. The changelog CI guard falls back to the frozen event payload rather than
   reading a failed lookup as 'no label'. Payload passed via env, never
   interpolated into the script body. Fallback logic tested locally across
   eight cases including malformed and null JSON.

* fix: close the four #155 follow-ups (#156)

1. _semver_key replaced by the semver library (new dep, zero transitive deps).
   Verified it rejects every case the hand-written regex was built to reject
   (leading zeros, empty identifiers, non-SemVer) and orders beta.2 < beta.11.

2. is_version_newer's docstring now warns that a bare `not is_version_newer(...)`
   is unsafe, and an AST-based test enforces it repo-wide -- proven to fire by
   removing the real guard in refresher.py.

3. All-numeric digest disambiguation. Fail-closed was tried and REVERTED: it
   broke CalVer, which #155 deliberately supports and pins with an existing
   test -- trading a live capability for a latent bug. The partner value
   disambiguates instead. Also fixed a real bug found on the way: the digest
   pattern demanded a hex letter even when the `sha256:` prefix was present,
   rejecting all-numeric prefixed digests.

4. The changelog CI guard falls back to the frozen event payload rather than
   reading a failed lookup as 'no label'. Payload passed via env, never
   interpolated into the script body. Fallback logic tested locally across
   eight cases including malformed and null JSON.

* fix: clear the sse_starlette AppStatus latch on entry, not only exit (#158)

Root cause of the intermittent test_ec_p2_7_reconnect_does_not_leak_transports
failure, confirmed empirically rather than inferred:

    latched=False: connect OK
    latched=True:  FAILED -> Exception: SSE stream ended without a response

which is the exact error CI reported.

AppStatus.should_exit is a process-global class attribute latched True by
uvicorn's shutdown handler and never reset. run_fake_remote cleared it in its
finally block, which protects against its OWN shutdown but not against a server
stopped elsewhere in the interpreter -- tests/mcp2x/test_listen_over_http.py and
tests/test_http_dos.py both stop uvicorn servers, and tests/mcp2x sorts
immediately before tests/runtime. That is why CI failed at ~3% of the suite and
why it never reproduced in isolation.

Clearing on entry makes the server immune to whatever ran before it. Added a
test that latches the flag deliberately and asserts a connection still works;
proven RED without the entry reset.

#158 suggested instrumenting the assertion to learn which descriptors were open.
That was the right next step for a socket-count failure, but the failures were
the SSE stream dying, not the count.

* fix: address all three board findings on #156

1. Partner promotion REVERTED. Promoting an all-numeric value to a digest
   because its partner is one resolves the ambiguity by guessing, and the guess
   fabricates: a genuine CalVer paired with a digest reported an update that
   never happened. That is precisely what is_version_newer's fail-closed
   contract exists to prevent. Mixed pairs stay incomparable; a new test pins
   that every caller passes package_type, which is what actually resolves it.
   The separate sha256:-prefix fix is retained -- that one was unambiguous.

2. The AST negation test checked only CO-OCCURRENCE. It now requires the guard
   to be a SIBLING operand of the and-chain, positioned BEFORE the negation, and
   about the SAME value. My first strengthening still accepted the reviewer's
   bypass, because walking into each operand finds a guard nested in an `or`.
   Verified against four bypasses: or-nesting, wrong subject, no guard, and
   guard-after-negation. All four now fail; real code passes.

3. The CI label fallback branched on EMPTINESS, conflating "lookup failed" with
   "lookup succeeded, zero labels". A removed skip-changelog label would have
   been resurrected from the frozen payload and silently bypassed the guard --
   worse than the false block the fallback was added to prevent. It now branches
   on exit status. Tested across five cases including label-removed.

* fix: make the negation guard model control flow, not syntax

Third attempt at this check, and both reviewers found the same remaining hole:

    (ready or is_version_orderable(x)) and not is_version_newer(x, y)

passed, because the guard was searched for RECURSIVELY inside each preceding
and-operand -- so a guard nested in an `or` counted, even though `ready` being
true skips it entirely. The previous version also REJECTED a genuinely safe
guard in an enclosing `if`, confirming it modelled syntax rather than effect.

Now computes the conditions that must actually hold where the negation is
evaluated: conjuncts of preceding operands in an enclosing `and` chain, plus
conjuncts of an enclosing `if` test when the negation is in its body (not its
orelse). An `or` contributes only itself, never its branches.

Verified against five bypasses (or-nested guard in a preceding operand,
`not newer or guard`, wrong subject, no guard, guard after the negation) and
four legitimate shapes (bool()-wrapped, parenthesised negation, enclosing `if`,
`if ready and guard`). Also rejects a guard that only holds in the `if`'s else
branch.

Plus the stale prose the reviewer flagged: the digest test's class docstring
still described the reverted partner-promotion design, the workflow comments
still described the replaced empty-check fallback, the CHANGELOG had picked up
a duplicate '### Fixed' heading, and its CI bullet omitted that a SUCCESSFUL
empty lookup stays authoritative.

* test: accept a walrus-assigned guard, document the early-exit limitation

Found while pre-attacking the review questions: (ok := is_version_orderable(v))
is truthy exactly when the guard is, so it should count -- NamedExpr is now
stripped alongside bool() and double negation.

An early-exit guard (if not is_version_orderable(v): return False) is genuinely
safe but still trips this check. Documented in the test rather than left for
someone to rediscover, with instructions to restructure or extend the analysis
rather than delete the assertion.

* fix: guard BOTH sides of the version comparison (#156 item 2, for real)

Board review proved the item-2 defect was still live after three rounds of
fixing the lint that was supposed to police it. The guard checked only the
CACHED side:

    is_version_orderable('1.0.0')             = True
    is_version_newer('1.0.0','nightly','npm') = False   (fails closed)
    guard and not newer                       = True    -> "up to date"

so an unreadable FETCHED version pinned the stale cache exactly as the original
bug did. Both sides are now checked, and both checks carry pkg_type -- the type
is what makes an all-numeric docker digest orderable at all.

Added a behavioural regression test asserting the observable (refresh_server
calls get_package_version a second time on the refresh path, so a short-circuit
is detectable), proven RED against the one-sided guard. The lint now requires
both operands guarded and reports which one is unguarded; also proven RED.

The lint remains unsound by construction -- it matches syntax, so it accepts a
guarded value that is reassigned before use, or a guard whose scope ended before
a closure runs. Documented in the test rather than left implicit, and filed as
#164: replacing the fail-closed boolean with a tri-state result
makes the hazard unrepresentable and this lint unnecessary.

* fix: comparability is a property of the PAIR, not of each value (#156 item 2)

Fifth board finding, and the deepest. Two unary orderability guards do not
prove a pair can be ordered:

    is_version_orderable('1.0.0', 'docker')        = True
    is_version_orderable('abcdef123456','docker')  = True
    is_version_newer('1.0.0','abcdef123456','docker') = False   (mixed, fails closed)
    -> guard passes, negation reports "up to date"

A version and a digest are each orderable alone but mutually incomparable, so
the short-circuit returned the stale npm cache for a server now configured for
docker. Reachable because refresh_all reuses a cache entry by server NAME and
never checks the package still matches.

Adds are_versions_comparable(current, latest, package_type), which mirrors
is_version_newer's own branching, and switches the refresher to it. The lint now
requires that pair predicate over the same two operands rather than unary
guards; both it and a new behavioural test are proven RED against the
two-unary-guard version.

is_version_orderable is no longer used in src/. Kept for genuinely unary
questions, but its docstring -- and is_version_newer's, which still advised
gating on it -- now say plainly that it is NOT sufficient to guard the negation.

The pair/unary drift test initially failed on ('abcdef123456',
'sha256:abcdef123456'): the same image in two spellings is comparable and
EQUAL, so neither direction is newer. My premise was wrong, not the code. It
now asserts the invariant that actually matters -- an incomparable pair is
never reported newer in either direction.

* test: enforce package_type in the guard, and test the dangerous drift direction

Board findings 6 and 7, both correct.

1. The lint compared only the first two operands, so a guard that DROPPED
   package_type passed while the comparator beside it used one. The type
   decides classification -- are_versions_comparable("202612180000","1.0.0") is
   True while the same pair as docker is incomparable -- so that guard would
   have recreated the unsafe short-circuit. All three operands must now match.

2. The drift test SKIPPED every pair the predicate accepts, so it could only
   catch "predicate rejects something orderable" (which suppresses updates). The
   dangerous direction is the opposite: predicate accepts a pair the comparator
   cannot order, which is exactly what lets `not is_version_newer(...)` read
   "up to date". Now tested over the same 28-value x 7-type corpus: an accepted
   pair must either order in one direction or be canonically the SAME value.

Writing (2) exposed two bugs in my own canonical-identity helper before it
passed, both of which reported drift that was not there:
  - comparing rendered strings, so 1.0 vs 1.0.0 (the same PEP 440 release)
    looked different;
  - then comparing full Version objects, so 1.0.0 vs 1.0.0+b looked different,
    when is_version_newer deliberately compares .public because build metadata
    is not a new release.

Proven RED by injecting drift into are_versions_comparable: the test names
('1.0.0','abcdef123456',None) as comparable-but-unorderable.